Review: the separator is re-supplied on only one side of the identifier

Verified against this branch (c42f96c) and a live PostgreSQL 16.12.

1. Blocking: the closing quote welds too, and INTO slips through

The diagnosis is right but it stops halfway. A delimited identifier is self-delimiting on both ends. The fix re-supplies a separator before the identifier and nothing after it, so "x"INTO still fuses:

SELECT "x"INTO evil FROM t   ->   SELECT xINTO evil FROM t

INTO is in _DML_DDL_KEYWORDS precisely because SELECT ... INTO new_table is a write that opens with SELECT. \bINTO\b does not match xINTO, so rule 4 goes blind in exactly the way rule 6 did.

On this branch:

(Run inside a transaction and rolled back. Each returned SELECT 1, and the created table held the source rows.)

One of those shapes is worse here than on main. SELECT"x"INTO evil FROM t is blocked on main, because the leading weld produced SELECTxINTO ... and rule 3 (opening keyword) rejected it incidentally. Re-supplying the leading separator restores a valid-looking SELECT opener while the trailing weld still hides INTO, so it now passes. That is a net new bypass introduced by this change.

2. Blocking: the same root cause reaches the row-lock rule

SELECT * FROM t AS"a"FOR SHARE       -> PASS   (parses and takes a share lock on PG 16.12)
SELECT * FROM t AS"a"FOR KEY SHARE   -> PASS

FOR UPDATE survives only because bare UPDATE is independently in the DML deny-list. FOR SHARE has no such backstop, so rule 5 is bypassable.

3. The stated rationale points at a regex that is not in the file

Both copies of the comment say a blanket space would "break the (?<!\.) lookbehind that keeps a qualified column from reading as a bare dangerous identifier". There is no (?<!\.) anywhere in sql_guard.py:

$ grep -rn '(?<!' packages/agami-core/src/sql_guard.py plugins/agami/lib/sql_guard.py
packages/agami-core/src/sql_guard.py:141:  # break the `(?<!\.)` lookbehind ...
plugins/agami/lib/sql_guard.py:141:        # break the `(?<!\.)` lookbehind ...

The only two hits are the comment describing itself. _DANGEROUS_FN_RE is \b-anchored with no lookbehind, so t."pg_sleep"(1) is blocked today under either design (. is already a word boundary). This comment is the load-bearing justification for the conditional, and a reader cannot check it against the code.

4. The negatives do not pin what the description says they pin

The description says the corpus makes "a future over-correction (a blanket space) fail just as loudly as the original gap". It does not. Replacing the conditional with an unconditional out.append(" ") and running the file:

253 passed in 0.88s

All 253 pass, including all 6 new qualified-name negatives. Nothing in the suite distinguishes the conditional design from the blanket one. So either the conditional is not actually required, or the case that requires it is missing. Worth settling before this lands, because the conditional is the reason the trailing side was left unhandled.

5. Minor: prev can read as empty when it is not

prev = out[-1][-1:] if out and out[-1] else "" peeks at the last element, and out.append("".join(buf)) can append "" for a zero-length identifier, which hides the real preceding char. Tracking the last emitted char in a scalar alongside out is easier to reason about than peeking at list structure. I could not build an exploit through this (the doubled-quote escape path absorbs the adjacent cases), so this is robustness rather than a defect.

6. Minor: SECURITY.md is left overclaiming

The description notes this falsifies SECURITY.md:44-45 ("bypasses hidden in string literals, comments, or double-quoted identifiers"), but the file is not touched. With the trailing weld still open, that line remains an overclaim after this merges.

A direction that checks out

Mirroring the existing conditional on the closing side closes all of the above with no false positives:

prev = out[-1][-1:] if out and out[-1] else ""
if prev.isalnum() or prev == "_":
    out.append(" ")
out.append("".join(buf))
nxt = sql[j] if j < n else ""
if nxt.isalnum() or nxt == "_":
    out.append(" ")
i = j

With that applied: all seven shapes above are blocked, t."current_user" still neutralizes to t.current_user (not t. current_user), "schema"."table" still to schema.table, and the file is 253 passed. Whatever shape the final fix takes, the corpus needs at least one case that actually fails under the wrong variant, or point 4 recurs on the next change here.

What is good here

Real bug, correctly root-caused down to the \b anchor rather than patched at the pattern level. Verified end to end against a live engine instead of test-only confidence. Both surfaces synced through sync-lib so they cannot drift. The \b-anchor explanation is the right thing to have written down, and it is what made the remaining half of the bug easy to find.

All six addressed in 5ef42fb. Finding 1 was correct and was the serious one — the previous commit introduced a bypass, and I've verified that independently before fixing.

1 + 2 — the closing quote welds too

Confirmed on PostgreSQL 16.12. SELECT "x"INTO evil FROM t creates a table holding the source rows, and the guard passed it. Also confirmed the net-new regression you identified:

Fix is the mirrored conditional you suggested. All seven are now in the red-team corpus.

3 — the phantom lookbehind

You're right, and it was worse than a stale reference: I carried that justification over from a different guard, so it described code that isn't in this file. Removed. The real justification is the lexer's own documented invariant, four lines above the loop:

"Neutralized spans collapse to a single space (never empty — welding tokens like SELECT/**/INTO → SELECTINTO would defeat the \b word boundaries below)."

The contract was already written down; the identifier branch was the one place not honouring it. That's checkable against the file, unlike what I'd written.

4 — the corpus now discriminates

This was the most useful finding. You were right that nothing distinguished the designs. Added test_neutralize_preserves_token_structure, which asserts the neutralized string rather than the gate's yes/no. Re-ran both wrong variants against the corpus:

blanket separator (both sides)  ->  5 failed, 261 passed
leading side only (prev commit) ->  9 failed, 257 passed
correct (conditional, both)     ->  266 passed

So the conditional is now pinned by something that actually fails without it. To answer the underlying question directly: no current rule distinguishes t.col from t. col, because everything here is \b-anchored. The justification is structural — the neutralizer's job is to remove hiding places without re-tokenizing, and a blanket space turns one qualified name into two tokens. Any future rule reasoning about qualification would inherit that. Since that's a design decision with no present behavioural signal, it's asserted on the output directly.

5 — prev peeking at list structure

Replaced with _last_emitted(), which walks back over empty chunks. Agreed it was robustness rather than a defect — the doubled-quote path absorbs the reachable cases — but the zero-length-identifier hole was real.

6 — SECURITY.md

Left as-is deliberately: with both ends closed, the claim about "bypasses hidden in … double-quoted identifiers" is now true rather than aspirational. "a;b" still reduces to a;b and trips the multi-statement rule (verified). The fix makes the doc accurate; editing it would have been the wrong direction.

python3 dev.py check green, mirrors byte-identical via sync-lib. Thank you for catching the trailing side — a partial fix that re-opens a shape main blocked is worse than no fix, and the mutation check in 4 is the thing that stops it recurring here.

Re-review of 5ef42fb: all six points addressed, verified

Re-ran everything against the new head.

The two blocking findings are closed

All nine shapes from the previous comment are now rejected, including the one that regressed against main:

BLOCKED  SELECT "x"INTO evil FROM t
BLOCKED  SELECT"x"INTO evil FROM t
BLOCKED  SELECT t."x"INTO evil FROM t
BLOCKED  SELECT 1 AS"a"INTO evil
BLOCKED  WITH c AS (SELECT 1 AS "x")SELECT "x"INTO evil FROM c
BLOCKED  SELECT * FROM t AS"a"FOR SHARE
BLOCKED  SELECT * FROM t AS"a"FOR KEY SHARE
BLOCKED  SELECT*FROM"pg_read_file"('/etc/passwd')
BLOCKED  SELECT 1 FROM"pg_class"WHERE"pg_sleep"(10) IS NULL

I also swept ten shapes that are not in the corpus, to check the fix generalizes rather than matching the reported cases. All blocked:

SELECT"a"INTO"evil"FROM t                                   -> SELECT a INTO evil FROM t
SELECT * FROM t AS"a"FOR NO KEY UPDATE                      -> SELECT * FROM t AS a FOR NO KEY UPDATE
SELECT 1 FROM t AS"a"WHERE"pg_read_file"('…')IS NULL        -> … WHERE pg_read_file( )IS NULL
SELECT"a"."b"INTO evil FROM t                               -> SELECT a.b INTO evil FROM t
WITH"c"AS(SELECT 1)SELECT"x"INTO evil FROM"c"               -> WITH c AS(SELECT 1)SELECT x INTO evil FROM c
SELECT * FROM"t"WHERE x="a;DROP SCHEMA public CASCADE"      -> multi-statement

The corpus now actually discriminates, which was the point of finding 4

This is the part I most wanted to see, and test_neutralize_preserves_token_structure does the job. I rebuilt both wrong designs against the new test file:

So the suite now fails loudly in both directions: over-correction trips the structural assertions, under-correction trips the red-team cases. Previously the blanket variant passed all 253. Pinning the neutralized string rather than only the gate verdict is the right call, and the docstring explaining why the gate-level tests cannot tell the designs apart is worth keeping.

Findings 3 and 5

The (?<!\.) reference is gone, and the replacement is honest about its own status: it now says the structural choice is pinned by an explicit output test "since no current rule distinguishes the two spellings on its own", and frames qualification-aware matching as a hypothetical future rule rather than an existing one. That is checkable against the code as written.

_last_emitted handles the empty-chunk case. I checked it does not open a performance path, since the backward scan runs inside the 50KB cap:

many empty idents    len=48008    22.23 ms
many quoted idents   len=50000    10.26 ms
welded idents        len=48007    13.80 ms
one huge ident       len=49009     2.43 ms

Linear, no blowup.

Withdrawing finding 6

I was wrong to leave SECURITY.md on the list. That line is only an overclaim while the trailing weld is open; with both directions closed it is accurate again, so there is nothing to change. Confirmed the multi-statement half specifically: SELECT 1 AS"a";SELECT 2 and SELECT * FROM"t"WHERE x="a;DROP SCHEMA public CASCADE" both trip the multi-statement check.

Gate status

1584 passed, 1 skipped        (full suite)
ruff check                    All checks passed!
diff packages/… plugins/…     vendored mirror identical

One thing I probed and cleared

The trailing check keys off sql[j], the raw source char, rather than the next emitted char. That is only safe if every other branch emits at least one non-word char. I tested the interleavings: block comment, line comment, string literal, dollar-quoted literal, empty identifier on either side, and doubled-quote escapes. All blocked.

The single shape that slips is SELECT "x"$1INTO evil FROM t (x$1INTO, no \b before INTO). It is pre-existing on main, unrelated to this change, and PostgreSQL 16 rejects it anyway:

ERROR:  trailing junk after parameter at or near "$1INTO"

So there is nothing to do here. Noting it only so the assumption is on the record as tested rather than assumed.

Two small nits, neither blocking

1. if _last_emitted(out).isalnum() or _last_emitted(out) == "_": calls the helper twice. The previous revision bound prev first, which read better and scanned once. Worth rebinding.

2. ruff format --check now reports these three files; on main all three come back "already formatted". CI stays green because dev.py lint runs format with allow_fail=True, but the "unformatted backlog" rationale for that does not apply to files that were clean before the PR. It is a blank line before the nested def plus quote-style normalization on two of the new test strings. One uvx ruff@0.15.19 format clears it.

Good fix. The root cause is now stated symmetrically, the corpus defends both edges of it, and the reasoning in the comments matches what the code actually does.